Cgroups V1 removal #464
Cgroups V1 removal #464
Conversation
|
✅ A new PR has been created in buildah to vendor these changes: containers/buildah#6499 |
|
Seems to be doing well in containers/podman#27551 . @containers/container-libs-maintainers PTAL. |
| } | ||
|
|
||
| // Create the cgroup. | ||
| func (c *linuxCpusetHandler) Create(ctr *CgroupControl) (bool, error) { |
There was a problem hiding this comment.
OK, removing this is one way to deal with #417 (comment) .
… but, please, make a permanent record of the rationale. Why is removing the …CopyFromParent code fine? The commit message doesn’t say. (I don’t understand cgroups and I have no opinion. It’s quite possible that the manual copies were a bug.)
If this should be obvious to anyone minimally familiar with cgroups, I’m happy to defer to a different reviewer.
common/pkg/cgroups/cpuset_linux.go
Outdated
| } | ||
|
|
||
| // Stat fills a metrics structure with usage stats for the controller. | ||
| func (c *linuxCpusetHandler) Stat(_ *CgroupControl, _ *cgroups.Stats) error { |
There was a problem hiding this comment.
If the cpuset handler should stay as is now, entirely empty struct with no code, it looks like it could be removed (…and, optionally, instead a “handler” interface, we could just have a collection of stat-gathering functions)
| } | ||
|
|
||
| // AddPid moves the specified pid to the cgroup. | ||
| func (c *CgroupControl) AddPid(pid int) error { |
There was a problem hiding this comment.
This passed in #417 without comment — it’s not trivially obvious to me that removing this without replacement is fine. It might well be obvious to someone more familiar with cgroups.
There was a problem hiding this comment.
Per containers/podman#27551 (comment), this needs to stay.
|
Packit jobs failed. @containers/packit-build please check. |
2 similar comments
|
Packit jobs failed. @containers/packit-build please check. |
|
Packit jobs failed. @containers/packit-build please check. |
This reverts commit 5273685 , re-enabling cgv1 removal. Signed-off-by: Lokesh Mandvekar <[email protected]>
…omParent The manual cpusetCopyFromParent function was needed for cgroups v1 to populate cpuset.cpus and cpuset.mems from parent cgroups. In cgroups v2, the kernel automatically handles cpuset inheritance through the cpuset.cpus.effective and cpuset.mems.effective interfaces. When fs2.NewManager().Set() is called with cpuset resources, it directly writes the values to cpuset.cpus and cpuset.mems if provided. If not provided, the kernel ensures child cgroups have valid effective cpusets by automatically inheriting from their parent. Signed-off-by: Lokesh Mandvekar <[email protected]>
The Update() function previously iterated through handlers calling their Apply() methods. With cgroups v1 removed, all handlers' Apply() implementations were identical - they all called fs2.NewManager().Set(). This commit removes the redundant handler Apply() methods and has Update() call fs2.NewManager().Set() directly. The Update() method now performs the same function more efficiently without the handler indirection. Signed-off-by: Lokesh Mandvekar <[email protected]>
The github.com/opencontainers/cgroups/fs import and associated struct fields (Blkio, CPU, CPUSet, Mem, Pid) were never used in any of the handler implementations. These were remnants from the cgroups v1 removal. The empty handler structs are retained as they serve as method receivers for implementing the controllerHandler interface. Signed-off-by: Lokesh Mandvekar <[email protected]>
The cpuset handler only had an empty Stat method that returned nil, providing no functionality. Remove the handler entirely along with the unused CPUset and CPUAcct controller constants which were remnants from cgroups v1. Signed-off-by: Lokesh Mandvekar <[email protected]>
Replace the controllerHandler interface with a simple statFunc type and convert all handler structs to plain functions. This eliminates unnecessary abstraction since all handlers only implemented a single Stat method. Changes: - Replace controllerHandler interface with statFunc type - Convert linuxBlkioHandler, linuxCPUHandler, linuxMemHandler, and linuxPidHandler structs to blkioStat, cpuStat, memoryStat, and pidsStat functions - Remove init() function and factory methods (getBlkioHandler, etc.) - Simplify handlers map to direct function references - Update Stat() method to call functions directly - Update AvailableControllers signature to use statFunc type Signed-off-by: Lokesh Mandvekar <[email protected]>
Signed-off-by: Lokesh Mandvekar <[email protected]>
Signed-off-by: Lokesh Mandvekar <[email protected]>
This reverts commit cce5ec8. Note: This has already gone through a revert and re-revert cycle in commits 5273685 and 3fe402b, but that's wrong per: containers/podman#27551 (comment) . Signed-off-by: Lokesh Mandvekar <[email protected]>
| } | ||
| return nil | ||
| path := filepath.Join(cgroupRoot, c.config.Path) | ||
| return fs2.CreateCgroupPath(path, c.config) |
There was a problem hiding this comment.
@giuseppe could you PTAL at this diff please? You mentioned AddPid is required, but if it's only cgroup2, then I think this could be simplified as the current diff, but in that case the pid isn't even being used. I'll defer to you.
3 participants
See individual commits.